feat(storage): add S3 binary backfill, starter export/import and integrity repair by swicken · Pull Request #37773 · dotCMS/core

swicken · 2026-09-28T18:29:33Z

S3 asset storage, part 4 of 7. Stacked PRs, review bottom up. Each one builds on the one below.
1a storage layer #37770 · 1b binary asset API #37771 · 2 content #37772 · 3 recovery #37773 · 4 publishing #37774 · 5 temporary uploads and WebDAV #37775 · 6 rendering #37776
Everything is behind FEATURE_FLAG_S3_ASSET_STORAGE, off by default. With the flag off, behavior matches main.

Rebased on 2026-10-06 onto a fix in #37772 (3523198625). This PR's own commits are unchanged; the test counts below were taken before that rebase.

Refs #37868

Proposed Changes

Backfill job (POST /api/v1/jobs/binaryAssetBackfill, administrators only, checked at queue and run time). Copies existing originals, metadata and recognized renditions to S3 in verified batches, advancing a persisted cursor only after a fully verified batch. Resumable and cancellable, never deletes local sources, and rejected while the flag is off. A row whose binary or metadata exists nowhere, or whose JSON cannot be read, is skipped and reported in the job result (skippedCount and up to 1000 skippedInodes); a storage error still fails the batch and names the inode.
Legacy Image and File values are included only when the exact physical object exists, so linked identifiers, URLs and other text are never mistaken for binaries.
Starter export and import. Export restores originals from S3, one cache lease per binary, and skips a binary over the size limit before restoring it. A failed export leaves an unreadable, truncated archive rather than a valid but incomplete ZIP. Import publishes binaries to S3 before the database commit. A binary or metadata record that is in neither the starter nor S3 is logged and skipped, as on main, so starters exported without assets, with maxSize or with oldAssets=false still import; S3 and database errors still fail the import, and so does an error importing rules, so a starter is never left partly imported. Importing over a populated database with the flag on performs a full replacement with foreign keys enforced.
Integrity repair. The file-asset integrity checker copies stored binaries to the repaired content before publishing its corrected JSON, and a rolled-back repair deletes the objects it uploaded.

Behavior with the flag off

Unchanged from main; the backfill processor does not register.

Review fixes

The last commit on this branch (fix(storage): bound recovery scans, tolerate missing starter binaries and fail exports unreadably) addresses a full review of this PR. All of it is flag-on only:

The backfill batches and the export paging now limit rows in the SQL. DotConnect.setMaxRows only trims rows after they are fetched, so each batch used to read the whole remaining contentlet table, and export loaded every row's JSON into memory. Export pages on inode and loads each row's JSON on its own.
Missing starter binaries are tolerated as described above. Before, any starter without every referenced binary failed to import, which also failed a flag-on first boot.
One unreadable row no longer stops the backfill job permanently, and the job sends a heartbeat per inode.
A failed export can no longer produce a well-formed partial ZIP.
The export lease is per binary rather than for the whole export, and over-size binaries are not downloaded.
A rolled-back integrity repair no longer leaves orphan revision objects.
The doc notes that a starter exported with the flag on cannot be restored into an instance with the flag off.

Rollback safety

Backfill converts raw S3 objects into SHA-256 references in place, which an older release cannot read. Same rule as 2: once the flag is on, treat it as forward-only.

Checklist

Tests: the 163 unit tests in the doc's run command pass after the review fixes (1 skip, STS), including the new BinaryAssetBackfillTest and ExportStarterFailureTest. Integration on this branch before the review fixes, flag off: 34 run, 0 failures, including the existing ContentFileAssetIntegrityCheckerTest and ESMappingAPINumericFieldTest; flag on: 23 run, 0 failures. BinaryAssetStarterRestoreTest (registered in Junit5Suite1, opt-in) passed all phases against one MinIO bucket before the review fixes: export, fresh-database restore from the exported ZIP, and replacement of a populated database. After the review fixes, the integration suites were run on the top of the stack (feat(storage): serve renditions, compiled CSS and templates through S3 asset storage #37776); the starter phases were not rerun.
Translations: not applicable.
Security Implications Contemplated: backfill requires an active administrator both when queued and when run. Populated-database import replaces content; the doc warns to run that test phase only in a disposable harness.

semgrep-dotcms · 2026-09-28T18:32:37Z

Semgrep found 12 CUSTOM_INJECTION-2 findings:

dotCMS/src/main/java/com/dotmarketing/util/starter/ImportStarterUtil.java
- L1137-1138 - Triage
- L1137-1138 - Triage
- L1137 - Triage
- L1148-1149 - Triage
- L1148-1149 - Triage
- L1148-1149 - Triage
- L1148 - Triage
- L1148 - Triage
dotCMS/src/main/java/com/dotmarketing/util/starter/ExportStarterUtil.java
- L840 - Triage
- L840 - Triage
dotCMS/src/main/java/com/dotcms/storage/binary/BinaryAssetBackfillProcessor.java
- L100-102 - Triage
- L100-101 - Triage

🟡 Medium severity issue identified in your code:

The method identified is susceptible to injection. The input should be validated and properly
escaped.

If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.

claude · 2026-09-28T18:32:54Z

Claude finished @jcastro-dotcms's task in 1m 52s —— View job

PR Review — S3 binary backfill, starter export/import, integrity repair

I reviewed the diff against origin/s3-stack/2-content, focusing on the new backfill job, the starter export/import paths, and the integrity-repair binary copy. All of this is flag-on only; with FEATURE_FLAG_S3_ASSET_STORAGE off the behavior matches main.

New Issues

🟠 High: dotCMS/src/main/java/com/dotmarketing/util/starter/ExportStarterUtil.java:544 — new Contentlet(APILocator.getContentletAPI().find(inode, systemUser, false)) dereferences a possibly-null find() result. When find() returns null for an inode listed in contentlet (deleted content type, broken version info, etc.), the copy constructor NPEs and aborts the entire starter export. The sibling backfill path already treats this exact condition as tolerable — BinaryAssetBackfill.readContent (BinaryAssetBackfill.java:184) checks found == null and skips the row — so the two code paths disagree on the same input. Guard it the same way (skip + warn, continue). This matches the still-open P1 from the earlier dotbot/github-actions review. Fix this →

Existing

🟡 Medium: dotCMS/src/main/java/com/dotcms/storage/binary/BinaryAssetBackfill.java:75 — rows.size() < limit marks the scan complete whenever a page comes back short. In the admin-triggered BinaryAssetBackfillProcessor (which runs against live traffic), a concurrent content delete between page fetches can shrink a page below limit while later inodes still exist. The processor then persists complete=true and stops, reporting success while content versions past the gap were never verified in S3. runAll() (starter import) is unaffected because it runs inside the import transaction. Confirm the page was truly the last by probing for any inode > cursor before declaring completion. Fix this →

Notes

The 16 Semgrep CUSTOM_INJECTION-2 findings on ImportStarterUtil (L1137/L1148, the ALTER TABLE ... TRIGGER statements), ExportStarterUtil (L840) and BinaryAssetBackfillProcessor (L100-102) are false positives: the trigger/table names come from the constant deletionTriggers map, not user input, and the backfill SQL uses bind parameters. No change needed, but they should be triaged/dismissed in Semgrep so they stop recurring.
The compare-and-set checkpoint in BinaryAssetBackfillProcessor.process (parameters JSONB merge gated on the prior afterInode), the for update row lock in copyInode, and the removeOnRollback listeners in ContentFileAssetIntegrityChecker.copyStoredBinaries are sound — idempotent-copy + verified-advance gives safe replay, and the rollback listeners correctly reclaim pre-commit S3 uploads.

Only the High finding is blocking; it's a one-line guard already written in the parallel path.

· s3-stack/3-recovery

semgrep-dotcms · 2026-09-28T19:35:59Z

Semgrep found 4 CUSTOM_INJECTION-2 findings:

dotCMS/src/main/java/com/dotcms/storage/binary/BinaryFieldCleanupProcessor.java
- L100-101 - Triage
- L110-111 - Triage
- L115-117 - Triage
- L115-116 - Triage

🟡 Medium severity issue identified in your code:

The method identified is susceptible to injection. The input should be validated and properly
escaped.

If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.

claude · 2026-09-28T19:46:57Z

test

claude · 2026-09-28T19:47:39Z

Pull Request Unsafe to Rollback!!!

Category: H-5 — Binary Storage Provider Change (with H-1 — One-Way Data Migration characteristics)
Risk Level: 🟠 HIGH
Why it's unsafe: This PR adds BinaryAssetBackfillProcessor/BinaryAssetBackfill, a new admin-triggered job that walks every contentlet row and copies its binary fields and metadata into the durable S3-backed storage chain used by the opt-in FEATURE_FLAG_S3_ASSET_STORAGE feature. The project's own docs (docs/testing/BINARY_S3_STORAGE.md, pre-existing section "Enabling the flag is not rollback-safe") already state that once a binary's active copy is recorded under the new .revisions/<uuid>/ key scheme in contentlet_as_json, neither a rollback to a prior release nor the same release with the flag turned back off can resolve those keys — the reader falls back to the legacy field-folder path and the binary "resolve[s] as stale or missing." This PR operationalizes that already-declared-unsafe conversion at scale: instead of only affecting content checked in while the flag happens to be on, an administrator can now run POST /api/v1/jobs/binaryAssetBackfill to proactively convert the entire existing asset inventory. ImportStarterUtil.deleteDotCMS() and getTablesToIgnore() also gain a flag-gated "full replacement" path that, on import, deletes contentlet, contentlet_version_info, variant, folder, language, and all workflow/template/rule/category/experiment tables that a flag-off import leaves untouched. Mitigating factors: the flag (FEATURE_FLAG_S3_ASSET_STORAGE) still defaults to false (AssetStorageFeature.java), so unaffected sites see no behavior change; and per the docs, the backfill specifically avoids rewriting contentlet_as_json storage paths and leaves local filesystem copies in place, which is safer than the plain check-in path. Risk applies only to sites that have explicitly opted into the flag and used the new backfill or full-replacement-import paths.
Code that makes it unsafe: dotCMS/src/main/java/com/dotcms/storage/binary/BinaryAssetBackfill.java (new, runAll/runBatch/copyInode), dotCMS/src/main/java/com/dotcms/storage/binary/BinaryAssetBackfillProcessor.java (new job processor), dotCMS/src/main/java/com/dotmarketing/util/starter/ImportStarterUtil.java (deleteDotCMS() bulk-restore trigger toggling and getTablesToIgnore() additions of contentlet, contentlet_version_info, variant, folder, language, workflow/template/rule tables), docs/testing/BINARY_S3_STORAGE.md ("## Migrating existing binaries (backfill)" section, describing the one-way nature).
Alternative (if possible): This already follows most of the two-phase pattern from the H-5 reference (local sources are left in place, DB paths for backfilled rows are not rewritten). To fully close the gap, provide a documented, supported downgrade path — e.g. a companion job or config flag that lets a rolled-back N-1 (or flag-off N) resolve .revisions/<uuid>/ keys written by normal check-in traffic while the flag was on, not just backfilled rows — before recommending customers run the backfill broadly in production.

AI rollback-safety analysis for commit range 3725042b3b20bfd7db805f735eed1b14bc8fb643..4ae1a5c8b546ec285782161a68a713ed642f9961.

github-actions · 2026-10-06T15:01:40Z

⚠️ AI review failed

Claude did not produce a review — the backend call errored before generating any output (provider: anthropic-bedrock, model: global.anthropic.claude-sonnet-5). This usually means the model has no Bedrock access grant in the target account, or the model ID is invalid — not a problem with this PR.

_{Run: #37481391284}

…grity repair Fourth slice of the S3 asset storage work. With FEATURE_FLAG_S3_ASSET_STORAGE on, administrators can migrate existing binaries to S3 through the checkpointed binaryAssetBackfill job, starter export restores originals from S3 and starter import publishes binaries before its database commit, and the file-asset integrity repair copies stored binaries with the corrected content. The backfill processor does not register while the flag is off, and flag-off behavior matches main.

… and fail exports unreadably These fixes apply only with FEATURE_FLAG_S3_ASSET_STORAGE on; flag-off export, import and error handling are unchanged. The backfill batch query and the starter export's content scan now put the page size in the SQL through DotConnect.setSQL(sql, limit). DotConnect.setMaxRows only trimmed rows after the driver had fetched the whole remaining table. The export now pages on inode alone and loads each accepted row's JSON separately, so a page never holds the table's JSON. Backfill and starter import share one policy for data problems a retry cannot fix: a referenced binary or metadata record that exists neither locally nor in S3, a row whose JSON cannot be parsed, and a legacy row whose content cannot be found are logged and skipped, and the rest of the row is still copied. Import logs a summary count, so starters exported without assets, with maxSize or with oldAssets=false import again, including on first boot. S3 and database errors still fail the batch or the import. The backfill job continues past skipped inodes and reports skippedCount and the first 1000 skippedInodes in its checkpoint and result; storage failures still fail the batch, and the job error now carries the failing inode. The checkpoint compare-and-set is unchanged. The job sends a heartbeat after every inode. A failed flag-on export no longer finishes the ZipOutputStream, so the client receives a truncated archive without a central directory instead of a valid partial backup. Export takes the cache lease per binary rather than for the whole export, skips a binary whose stored metadata exceeds maxSize before restoring it, and holds one lease only around the local folder walk, which reads already-local files. The file-asset integrity repair registers rollback listeners that delete the revision objects and metadata it uploaded if the repair transaction rolls back; a failed deletion is logged. The behavior doc describes these changes and states that a starter exported with the flag on cannot be fully restored into a flag-off instance.

github-actions · 2026-10-07T20:54:07Z

+                if (!filter.accept(new File(ConfigUtils.getAssetPath(), inodePath))) {
+                    continue;
+                }
+                final Contentlet content = new Contentlet(APILocator.getContentletAPI().find(inode, APILocator.systemUser(), false));


🔴 [P1] ExportStarterUtil.java:544 guard null find before copy

Current code:

final Contentlet content = new Contentlet(APILocator.getContentletAPI().find(inode, APILocator.systemUser(), false));

Problem: find() can return null, copy then NPEs.

Fix:

final Contentlet found = APILocator.getContentletAPI().find(inode, APILocator.systemUser(), false); if (found == null) { Logger.warn(this, "Skipping unreadable content " + inode); continue; } final Contentlet content = new Contentlet(found);

github-actions · 2026-10-07T20:58:13Z

+            binaries += copyInode(inode, skipped);
+            cursor = inode;
+            afterEachInode.run();
+        }


🟡 [P2] BinaryAssetBackfill.java:74 concurrent deletes can end the scan prematurely with complete=true

Current code:

return new Result(cursor, binaries, rows.size() < limit, List.copyOf(skipped));

Problem: The batch is marked "complete" whenever a page returns fewer rows than limit, but a concurrent content deletion between page fetches can shrink a page below limit while later inodes still exist.

Fix:

final boolean lastPage = rows.size() < limit && new DotConnect() .setSQL("select 1 from contentlet where inode > ?", 1) .addParam(cursor).loadObjectResults().isEmpty(); return new Result(cursor, binaries, lastPage, List.copyOf(skipped));

Why it matters: the admin-triggered BinaryAssetBackfillProcessor job runs against live traffic. If a page comes up short because rows were deleted, the job persists complete=true in its checkpoint and stops, so remaining content versions are never verified in S3, yet the job reports success. Starter import (runAll) is unaffected because it runs inside the import transaction.

github-actions · 2026-10-09T18:55:43Z

dotbot code review:

Reviewer: meta/muse-spark-1.3 (medium)
Overall: patch is incorrect
New findings this run: 0
Prior unresolved dotbot findings still relevant: 2
Active findings total: 2

No new actionable bugs were found in the current changes, but 2 prior unresolved dotbot findings still apply, so the patch remains incorrect.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · meta/muse-spark-1.3 · medium}

github-actions · 2026-10-09T19:00:04Z

dotbot code review:

Reviewer: ~z-ai/glm-latest (medium)
Overall: patch is incorrect
New findings this run: 0
Prior unresolved dotbot findings still relevant: 2
Active findings total: 2

No new actionable bugs were found in the current changes, but 2 prior unresolved dotbot findings still apply, so the patch remains incorrect.

Tip: comment with "/dotbot address comments" to attempt automated fixes for unresolved review threads.

_{reviewed by dotbot · ~z-ai/glm-latest · medium}

github-project-automation Bot added this to dotCMS - Product Planning Sep 28, 2026

swicken force-pushed the s3-stack/3-recovery branch from 05b261e to 4ae1a5c Compare September 28, 2026 19:32

claude Bot added the AI: Not Safe To Rollback label Sep 28, 2026

swicken force-pushed the s3-stack/3-recovery branch from 4ae1a5c to 86992b7 Compare September 29, 2026 14:26

github-actions Bot removed the AI: Not Safe To Rollback label Sep 29, 2026

claude Bot added the AI: Safe To Rollback label Sep 29, 2026

swicken force-pushed the s3-stack/3-recovery branch from 86992b7 to 4f778fe Compare September 29, 2026 19:32

swicken force-pushed the s3-stack/2-content branch from 165a660 to 5824a30 Compare September 29, 2026 19:32

github-actions Bot removed the AI: Safe To Rollback label Sep 29, 2026

claude Bot added the AI: Safe To Rollback label Sep 29, 2026

swicken mentioned this pull request Oct 2, 2026

Store binary assets durably in S3-compatible object storage behind a feature flag #37868

Open

3 of 19 tasks

swicken force-pushed the s3-stack/2-content branch from 5824a30 to ea2656b Compare October 2, 2026 15:54

swicken force-pushed the s3-stack/3-recovery branch from 4f778fe to f552bdb Compare October 2, 2026 15:54

github-actions Bot removed the AI: Safe To Rollback label Oct 2, 2026

swicken force-pushed the s3-stack/2-content branch from ea2656b to f874dda Compare October 2, 2026 16:31

swicken force-pushed the s3-stack/3-recovery branch from f552bdb to 13879e9 Compare October 2, 2026 16:31

swicken marked this pull request as ready for review October 2, 2026 18:24

swicken force-pushed the s3-stack/3-recovery branch from 13879e9 to 6771ed3 Compare October 6, 2026 14:43

nollymar added the PR : dotbot review Trigger dotbot AI code review and the post-merge QA test plan label Oct 6, 2026

swicken added 2 commits October 7, 2026 16:25

swicken force-pushed the s3-stack/3-recovery branch from 6771ed3 to ed86935 Compare October 7, 2026 20:47

swicken force-pushed the s3-stack/2-content branch from 3523198 to 236a0d2 Compare October 7, 2026 20:47

github-actions Bot reviewed Oct 7, 2026

View reviewed changes

claude Bot added the AI: Safe To Rollback label Oct 7, 2026

github-actions Bot reviewed Oct 7, 2026

View reviewed changes

Merge branch 's3-stack/2-content' into s3-stack/3-recovery

3733680

github-actions Bot removed the AI: Safe To Rollback label Oct 9, 2026

Navigation Menu

Repository navigation

feat(storage): add S3 binary backfill, starter export/import and integrity repair - #37773

feat(storage): add S3 binary backfill, starter export/import and integrity repair#37773
swicken wants to merge 3 commits into
s3-stack/2-contentfrom
s3-stack/3-recovery

swicken commented Sep 28, 2026 •

edited

Loading

Uh oh!

semgrep-dotcms Bot commented Sep 28, 2026

Uh oh!

claude Bot commented Sep 28, 2026 •

edited

Loading

Uh oh!

semgrep-dotcms Bot commented Sep 28, 2026

Uh oh!

claude Bot commented Sep 28, 2026

Uh oh!

claude Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

Uh oh!

github-actions Bot Oct 7, 2026

Uh oh!

github-actions Bot Oct 7, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

swicken commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Behavior with the flag off

Review fixes

Rollback safety

Checklist

Uh oh!

semgrep-dotcms Bot commented Sep 28, 2026

Uh oh!

claude Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review — S3 binary backfill, starter export/import, integrity repair

New Issues

Existing

Notes

Uh oh!

semgrep-dotcms Bot commented Sep 28, 2026

Uh oh!

claude Bot commented Sep 28, 2026

Uh oh!

claude Bot commented Sep 28, 2026

Uh oh!

github-actions Bot commented Oct 6, 2026

⚠️ AI review failed

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 7, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

github-actions Bot commented Oct 9, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swicken commented Sep 28, 2026 •

edited

Loading

claude Bot commented Sep 28, 2026 •

edited

Loading